I'm curious, do you find something deficient in #151 that motivated the creation of this PR? StateWatcher is never entered in a HashMap, nor compared with other instances of StateWatcher in any way, so equals() and hashCode() won't have any effect. I agree it's a little strange the way these StateWatcher instances "float free", as opposed to being tracked in a map or something (directly, at least -- as of #151 they are effectively "tracked" indirectly via collectionWatches). But for better or worse it seems to be idiomatic to how things are done currently in ZkStateReader, and I'm confident in the fix proposed in #151.

I'm curious, do you find something deficient in #151 that motivated the creation of this PR? StateWatcher is never entered in a HashMap, nor compared with other instances of StateWatcher in any way, so equals() and hashCode() won't have any effect. I agree it's a little strange the way these StateWatcher instances "float free", as opposed to being tracked in a map or something (directly, at least -- as of #151 they are effectively "tracked" indirectly via collectionWatches). But for better or worse it seems to be idiomatic to how things are done currently in ZkStateReader, and I'm confident in the fix proposed in #151.

Statewatcher is tracked at zkclient level in hashmap. Now with equal and hashcode we will only one statewatcher at client. I would run your test case and observe the difference

@hiteshk25 interesting! I have never thought of controlling it in a lower level with such a simple change! 😊

I agree with that the existing code is actually odd as @magibney pointed out (free floating StateWatcher, hence the "passive" removal). Even with this change, I raised the same question of how does this solution work - It does take quite a bit of digging into the code (and not entirely obvious at all!)... that ZKWatchManager keeps several maps for each type of watcher (children, data, exists etc), and upon finishing a packet, it would register the corresponding watch. There is a final hashSet of watchers there, so i assume a watcher (wrapped as ProcessWatchWithExecutor 😓 ) which hashcode/equals relies on watched collection string will be ignored if there's already one there

Considering the scope of a single ZkStateReader instance, this proposed fix should also work, cause after all this StateWatcher reads and processes the collectionWatches of the outer class instance anyway, it really does not make sense to have multiple StateWatcher instances handling events for the same reader.

On the other end, if there are multiple ZkStateReader instances that uses the same zkClient instance, then we could have some issues. I'm not entirely sure about the design of ZkStateReader (is it meant to be singleton per collection?) as the documentation seems to be lacking 😓

ha, sure enough! This is actually leaning on ZooKeeper code (nothing in Solr) to dedupe these watches.

Honestly I'd be inclined to add hashCode() and equals() to #151 , rather than exclusively relying on the lower level to catch this. Afaict from tracing the code, an approach that relies on equals() and hashCode() to dedupe only does so after rtx communication to the zk server (though this is no different from the situation in #151), and from what I can tell it looks like it is neither expected nor general practice to override equals() and hashCode() on zk Watcher implementations.

In this case it's the right thing to do, but I'm still thinking through the implications, and whether it makes sense alone or complementary to the changes in #151.

On the other end, if there are multiple ZkStateReader instances that uses the same zkClient instance, then we could have some issues

^ this. The "gotcha" here is: currently, within the scope of a ZkStateReader, StateWatcher instances are intended to be singleton-per-collection. This is quite transparent (if not explicit) from the way it's used and the way it keys off of the ZkStateReader.collectionWatches map. Since ZkStateReader provides public ctor that accepts a supplied zkClient, I think the public ctor makes the choice for us. There might be plugins, etc. that follow this pattern (why, who knows? should they? probably not).

But aside from 3rd-party backcompat concerns, IMO the singleton logic is clear when the deduping is kept directly within the ZkStateReader code. Implementing hashCode() and equals() is so far removed from where the deduping logic is actually applied (in zookeeper project code) that it's very unclear what scope hashCode() and equals() dedupe at.